Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Tuning GetNeighbors perf #103

Closed
wants to merge 8 commits into from
Closed

Tuning GetNeighbors perf #103

wants to merge 8 commits into from

Conversation

critical27
Copy link
Contributor

@critical27 critical27 commented Aug 5, 2020

Better to review by each commit, some modification is scattered among lots of files.

Some major improvement:

  1. Only retrieve srcId/type/rank/dstId in collectEdgeProps if needed
  2. Only add FilterNode and AggregateNode if necessary.
  3. Add FLAGS_enable_multi_versions, there is a string copy if we want to filter the edge of different versions.
  4. Allocate RowReader from stack instead of heap, similar to fix VertexHolder::getDefaultProp performance issue. nebula#2249. The difference is that we use RowReaderWrapper to wrap reader of v1 and v2.

The Benchmark will retrieve 1 tag and 1000 edges, almost the same as vesoft-inc/nebula#2270, as follows:

  1. Normal processor of GetNeighbors, one vertex, no filter.
  2. Normal processor of GetNeighbors, one vertex, with filter. filter_ratio could modify the ratio of edges which could pass the filter.
  3. Only use a SingleEdgeNode to get 1000 edges.
  4. Directly use kvstore to get 1 tag and 1000 edges.

After these tuning, the benchmark of 2.0 has ~20% improvement than 1.0.

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Aug 5, 2020
Copy link
Contributor

@bright-starry-sky bright-starry-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me

src/codec/test/RowReaderV2Test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

src/codec/RowReaderWrapper.cpp Show resolved Hide resolved
panda-sheep
panda-sheep previously approved these changes Aug 6, 2020
@bright-starry-sky
Copy link
Contributor

redundant diff file.

@critical27
Copy link
Contributor Author

redundant diff file.

Good catch, thx.

@critical27 critical27 closed this Aug 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants